-
Notifications
You must be signed in to change notification settings - Fork 2.5k
SwiftTesting: Include metadata for tags, bug, and time limit traits in event stream #3040
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ad4afed to
bf363fb
Compare
| ### Enhanced Capabilities | ||
| With access to this metadata, tools can now offer features like: | ||
| - Filtering test runs by tags for faster feedback cycles | ||
| - Generating reports that group results by test categories | ||
| - Tracking performance regressions against defined time limits | ||
| - Automatically linking test failures to relevant bug reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section feels very repetitive with the "Immediate Benefits for Tool Developers" section, can they be consolidated or remove this section if it's redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
|
|
||
| - **ABI Version Protection**: New fields are conditionally included based on ABI | ||
| version checks, ensuring older tools continue to function without modification | ||
| - **Experimental Feature Migration**: The existing experimental `_tag` field is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current experimental field is named _tags, plural
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
| replaced with the `tags` array. Since experimental features don't provide | ||
| stability guarantees, this replacement doesn't constitute a breaking change | ||
| - **Graceful Degradation**: Tools that don't expect the new fields will simply ignore | ||
| them, while updated tools can leverage the enhanced metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!
| - **Flattened vs Structured Bug Information**: We chose a structured approach for bug | ||
| metadata to accommodate various bug tracking systems while maintaining extensibility | ||
|
|
||
| ### Implementation Approaches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ### Implementation Approaches | |
| ### Unconditionally include optional fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
|
|
||
| ### Potential Extensions | ||
| - **Additional Metadata**: Other test traits could be exposed as the ecosystem evolves | ||
| - **Enhanced Bug Integration**: More sophisticated bug tracking integration with status updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one feels pretty tentative to me. IMO "Future directions" don't need to be planned or expected; they may never happen. But I think they should be plausible and feel like a natural evolution of the product. It's certainly subjective, but if something like this were to be added I would expect it to be part of some related tool rather than an evolution of Swift Testing itself. If you agree, I'd suggest removing this bullet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I completely agree. I feel the Bug trait quite "limiting" in terms of name as I may want to add a trait that indicates what "Issues" a test validate/verifies. So, I can see something like .issue(issueId:relationship:) trait that could looks something like this:
@Test(
.issue(https://github.com/organization/project/issue/123456", relationship: .verifies),
)
I understand the name .issue may be contentious as it is likely to collide with Issue representing an Test "issue", but all this to say I would like to see an evolution to the Bug trait such that it becomes a subset of this envisioned Issue (name TBD) trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually ended up removing this and the Performance analytics bullet.
|
|
||
| The new fields are conditionally included in the JSON output based on test | ||
| configuration: | ||
| - Fields are only included when the test actually uses the corresponding traits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields are only included when the test actually uses the corresponding traits
The word "uses" is a bit confusing to me, since some traits can be applied to a test but may not be used by that test in any meaningful way. I think a better word choice would be "applied" or "applied to", as in:
| - Fields are only included when the test actually uses the corresponding traits | |
| - The field corresponding to each trait is only included for a given test if the | |
| trait has been applied to the test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing the subsequent bullets, I think this section could become a (shorter) paragraph without a title:
The field corresponding to each trait is only included for a given test conditionally, if the
trait has been applied to the test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates the bulltet point and removed the "paragraph section"
| - Empty or unused traits result in omitted fields, keeping the JSON clean and | ||
| efficient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bullet doesn't seem to add much to the previous one, especially if you take my suggestion.
Plus, I don't know of a way to specify an "empty" version of any of these traits; they all require at least one value (at least one tag, for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, I removed it.
| - Fields are only included when the test actually uses the corresponding traits | ||
| - Empty or unused traits result in omitted fields, keeping the JSON clean and | ||
| efficient | ||
| - This approach maintains backward compatibility while providing rich information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the versioning of the event stream which ultimately preserves backwards compatibility; clients that need to maintain compatibility can choose to continue using an older version until they're ready to upgrade.
I commented separately, but I think this bullet could be removed and you could consolidate this group of bullets into a sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
| - **`tags`**: An array of string tags associated with the test, enabling categorization | ||
| and filtering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this proposal should clearly specify what format the string representation of each tag will have in the JSON. For example, consider this test:
@Test(.tags(.foo, .MyTagGroup.bar, .MyTagGroup.Subgroup.baz))
func example() { ... }It has three tags of varying styles: .foo, .MyTagGroup.bar, and .MyTagGroup.Subgroup.baz. The handling of the leading period (.) character can sometimes be an important detail to specify and establish clarity around because in order for a supporting tool to integrate properly, it may need to precisely align these representations. There can also sometimes be odd details to contend with: it's technically valid syntax to have extraneous whitespace or /* ... */ comments between the tag components, for example.
There are several potential ways one could represent these different tags in the JSON: as a flattened string, with or without leading period, or decomposed into discrete "component" arrays perhaps. There are pros/cons to each approach.
I recommend you include a code example of tags, and the other traits, in this proposal, and also include an example of precisely how those traits would be represented in the JSON. And for tags specifically, I would be more verbose about the handling of the period separator in this bullet and also in the BNF-formatted JSON grammar. (That will require settling on an answer to the question above.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I augmented the BNF output and included a sample JSON.
I did not explicitly mention the "." in the tag name, though it should be clear with the updated BNF.
| ### Ecosystem Growth | ||
| By providing this metadata, we anticipate growth in the Swift Testing tooling | ||
| ecosystem, with new tools emerging to take advantage of the richer data available. | ||
| This proposal positions Swift Testing as a platform that truly enables innovative | ||
| testing tools and workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section doesn't feel like a specific "future direction" to me. It feels more like a justification for the proposal, but the Motivation section already gets to that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
| + ["tags": <array:tag>,] ; the tags associated with this test function | ||
| + ["bugs": <array:bug>,] ; the bugs associated with this test function | ||
| + ["timeLimit": <timeLimit>] ; the time limits associated with this test function | ||
| + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The extra empty newline isn't necessary, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -0,0 +1,178 @@ | |||
| # Enhance Swift Testing JSON ABI with Rich Test Metadata | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having reviewed the proposal in detail now, I think the title could be more clear. It seems like the focus is adding a representation of three built-in traits to the event stream JSON. I'm going to suggest a title below. Feel free to consider it a starting point and edit/refine further, but I'll explain why I prefer it below:
| # Enhance Swift Testing JSON ABI with Rich Test Metadata | |
| # Include metadata for tags, bug, and time limit traits in event stream |
This suggested title:
- Uses the word "include" to signify that it's adding content, rather than "enhance". (We could choose the verb "add", instead.)
- Lists the specific things it's including (luckily, the words are fairly short) and prefers that over less precise words like "Rich Test Metadata" which is more open to interpretation (what qualifies as 'rich'?).
- Omits the product name ("Swift Testing"); the
ST-NNNNprefix will convey that already. - Avoids the word ABI; I think the event stream JSON is what is being supplemented; the binary interface (ABI) of the library isn't being modified. (I chose to omit the word 'JSON'; I briefly added it at the end of my suggested title, and you could add it back if you prefer.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
8a7e8b1 to
e7325c8
Compare
e7325c8 to
55d172e
Compare
55d172e to
f56f077
Compare
This proposal adds test metadata (
tags,bugsandtimeLimit) to Swift Testing's event JSON ABI, enabling richer external tooling capabilities.Links: Implementation | Forum Pitch